feat(hgraph): restore conjugate graph enhancement - #2675
Conversation
|
/label status/waiting-for-review |
Merge Protections🟢 All 3 merge protections satisfied — ready to merge. Show 3 satisfied protections🟢 Require kind label
🟢 Require version label
🟢 Require linked issue for feature/bug PRs
|
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Restores and integrates the HGraph conjugate-graph enhancement end-to-end (parameters, feedback/pretrain APIs, search-time result enhancement, and binary/streaming serialization), plus updates example and EN/ZH docs.
Changes:
- Added HGraph build/search parameters to enable conjugate-graph construction and toggled search-time usage.
- Implemented
Feedback/Pretrain/UpdateIdconjugate-graph integration, including streaming/binary serialization and memory accounting. - Migrated the C++ runnable example and synchronized English/Chinese documentation for the feature.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_hgraph.cpp | Adds functional coverage for conjugate-graph feedback, ID updates, and serialization roundtrips. |
| src/storage/serialization_tags.h | Introduces a new streaming block tag for conjugate-graph payloads. |
| src/impl/conjugate_graph.h | Adds StreamWriter-based serialization API for ConjugateGraph. |
| src/impl/conjugate_graph.cpp | Implements StreamWriter serialization for conjugate-graph state and footer. |
| src/algorithm/hgraph/hgraph_serialize.cpp | Serializes/deserializes conjugate-graph in binary + streaming formats and exposes metadata/memory usage. |
| src/algorithm/hgraph/hgraph_search.cpp | Enhances search results using conjugate-graph edges when enabled. |
| src/algorithm/hgraph/hgraph_parameter_test.cpp | Adds unit tests for parameter mapping and search parameter parsing. |
| src/algorithm/hgraph/hgraph_parameter.h | Adds new build/search parameter fields for conjugate-graph enablement and search toggle. |
| src/algorithm/hgraph/hgraph_parameter.cpp | Parses/serializes the new parameters and includes them in compatibility checks. |
| src/algorithm/hgraph/hgraph_param_mapping.cpp | Maps external param key(s) for conjugate-graph into internal HGraph config. |
| src/algorithm/hgraph/hgraph_enhance.cpp | Implements Feedback, Pretrain, and UpdateId conjugate-graph wiring. |
| src/algorithm/hgraph/hgraph.h | Adds conjugate-graph fields/mutex and new public API overrides. |
| src/algorithm/hgraph/hgraph.cpp | Initializes conjugate-graph based on build params. |
| src/algorithm/hgraph/CMakeLists.txt | Adds the new enhancement implementation unit to the build. |
| examples/cpp/304_feature_enhance_graph.cpp | Updates the example to HGraph + new parameter structure. |
| docs/docs/zh/src/resources/index_parameters.md | Documents use_conjugate_graph and use_conjugate_graph_search (ZH). |
| docs/docs/zh/src/indexes/hgraph.md | Documents the new build parameter (ZH). |
| docs/docs/zh/src/advanced/enhance_graph.md | Updates enhancement docs to HGraph APIs and new JSON structure (ZH). |
| docs/docs/en/src/resources/index_parameters.md | Documents use_conjugate_graph and use_conjugate_graph_search (EN). |
| docs/docs/en/src/indexes/hgraph.md | Documents the new build parameter (EN). |
| docs/docs/en/src/advanced/enhance_graph.md | Updates enhancement docs to HGraph APIs and new JSON structure (EN). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
LHT129
left a comment
There was a problem hiding this comment.
Code review for feat(hgraph): restore conjugate graph enhancement
09c0a4d to
98d83af
Compare
LHT129
left a comment
There was a problem hiding this comment.
Automated review by vsag-pr-review-agent
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/algorithm/hgraph/hgraph_search.cpp:754
- In the conjugate-graph enhancement path, results are converted inner_id -> label and then label -> inner_id again. When duplicate external IDs are enabled, LabelTable can have multiple inner IDs for the same label, and TryGetIdByLabel() returns an arbitrary one. That can cause mismatched (distance, inner_id) pairs (distances from one duplicate but extra_info/inner_id from another) when pushing back into search_result.
while (not label_results.empty()) {
const auto record = label_results.top();
label_results.pop();
const auto [found, inner_id] = this->label_table_->TryGetIdByLabel(record.second, true);
if (found and (ft == nullptr or ft->CheckValid(inner_id))) {
examples/cpp/304_feature_enhance_graph.cpp:67
- If CreateIndex() fails, hgraph remains null but is still dereferenced (hgraph->Build), which will crash the example. The example should exit/return on create failure and ideally surface the factory error message.
std::shared_ptr<vsag::Index> hgraph;
if (auto index = vsag::Factory::CreateIndex("hgraph", hgraph_build_parameters);
index.has_value()) {
hgraph = index.value();
} else {
98d83af to
4844741
Compare
LHT129
left a comment
There was a problem hiding this comment.
Review Summary
This PR restores conjugate graph enhancement for HGraph with comprehensive coverage: Feedback/Pretrain methods, search-time enhancement integration, UpdateId support, memory accounting, and both binary and streaming serialization. The implementation is well-structured and the test coverage is thorough.
Issues from previous review rounds
All previously reported issues have been addressed:
- Lock ordering:
label_lookup_mutex_is consistently acquired beforeconjugate_graph_mutex_across all code paths (Feedback,Pretrain,UpdateId, search enhancement). - Filter semantics: The enhancement result rebuild now holds a shared label lock and re-checks
ft->CheckValid(inner_id)before pushing results back. - Performance:
flattenandcomputerare created once outside the per-label lambda. - Streaming deserialization: Now correctly rejects a conjugate-graph block when the target HGraph was created without the feature, matching binary deserialization behavior.
- Typo fix: "constains" → "contains" in the example.
Current assessment
The code is clean, well-tested, and all critical concerns from earlier rounds have been resolved. No new issues found in this revision.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
src/impl/conjugate_graph.cpp:185
- The streaming format writes adjacency entries without an explicit
conjugate_graph_entry count, which makes the payload harder to evolve and validate (the reader must infer termination via footer mechanics). Consider serializing the number of nodes/entries up front (e.g.,conjugate_graph_.size()) so deserialization can be bounded and format changes are easier to manage.
StreamWriter::WriteObj(out_stream, memory_usage_);
for (const auto& [tag_id, neighbor_ptr] : conjugate_graph_) {
StreamWriter::WriteObj(out_stream, tag_id);
uint64_t neighbor_set_size = neighbor_ptr->size();
StreamWriter::WriteObj(out_stream, neighbor_set_size);
for (const auto neighbor_tag_id : *neighbor_ptr) {
StreamWriter::WriteObj(out_stream, neighbor_tag_id);
}
}
src/impl/conjugate_graph.cpp:190
- Serializing the footer through a temporary
std::stringstreamforces an extra buffering + string copy (str()), which can be noticeable for large serialized payloads or frequent serialization. Consider adding aFooter::Serialize(StreamWriter&)(or writing the footer into a pre-sized buffer once) to avoid the extra allocation/copy.
std::stringstream footer_stream(std::ios::in | std::ios::out | std::ios::binary);
footer_.Serialize(footer_stream);
const auto footer_data = footer_stream.str();
out_stream.Write(footer_data.data(), footer_data.size());
}
src/algorithm/hgraph/hgraph_search.cpp:754
- The
label_lookup_mutex_shared-lock is held across potentially expensive work (flatten->Query(...)andEnhanceResult(...)). This can increase lock contention and delay concurrent operations that need the label table (e.g., updates). Consider narrowing the lock scope: extract/copy the needed label mappings (or the initial label list) under the lock, then release it before computing distances and enhancing results; reacquire only for the finalTryGetIdByLabelre-mapping (or precompute those mappings too).
std::shared_lock label_lock(this->label_lookup_mutex_);
while (not search_result->Empty()) {
const auto record = search_result->Top();
search_result->Pop();
label_results.emplace(record.first, this->label_table_->GetLabelById(record.second));
}
const auto flatten = use_custom_distance ? nullptr : this->get_precise_codes();
const auto computer = use_custom_distance ? nullptr : flatten->FactoryComputer(raw_query);
const auto distance_of_label = [&](int64_t label) {
const auto [found, inner_id] = this->label_table_->TryGetIdByLabel(label, true);
if (not found or (ft != nullptr and not ft->CheckValid(inner_id))) {
return std::numeric_limits<float>::max();
}
float distance = std::numeric_limits<float>::max();
if (use_custom_distance) {
request.distance_batch_func_(&label, 1, &distance);
} else {
flatten->Query(&distance, computer, &inner_id, 1, &ctx);
}
if (request.threshold_.has_value() and distance > request.threshold_.value()) {
return std::numeric_limits<float>::max();
}
return distance;
};
{
std::shared_lock graph_lock(this->conjugate_graph_mutex_);
(void)this->conjugate_graph_->EnhanceResult(label_results, distance_of_label);
}
while (not label_results.empty()) {
const auto record = label_results.top();
label_results.pop();
const auto [found, inner_id] = this->label_table_->TryGetIdByLabel(record.second, true);
if (found and (ft == nullptr or ft->CheckValid(inner_id))) {
search_result->Push(record.first, inner_id);
}
}
examples/cpp/304_feature_enhance_graph.cpp:69
- If
CreateIndex(...)fails, the example continues and will dereferencehgraphlater (e.g.,hgraph->Build(...)), which will crash. The example should exit/return on failure (or throw) right after printing the error.
std::shared_ptr<vsag::Index> hgraph;
if (auto index = vsag::Factory::CreateIndex("hgraph", hgraph_build_parameters);
index.has_value()) {
hgraph = index.value();
} else {
std::cout << "Create HGraph Error" << std::endl;
}
LHT129
left a comment
There was a problem hiding this comment.
经过对最新 commit (3311525) 的全面审查,确认之前 Copilot 和其他 reviewer 发现的所有重要问题都已在修正后的代码中得到解决:
已确认修复的问题:
- 锁顺序问题 —
Feedback中现在正确使用label_lookup_mutex_(shared) →conjugate_graph_mutex_(unique) →memory_usage_mutex_(unique) 的顺序 - 过滤语义 — conjugate graph 增强后重新检查
is_allowed(inner_id)再 push 回结果 - flatten/computer 性能 — 在 lambda 外部创建,避免重复构造
- label-table 无锁访问 —
Pretrain中已使用TryGetIdByLabel并持有label_lookup_mutex_ - streaming deserialization 一致性 — 现在 streaming 路径也拒绝 mismatched conjugate graph 块
- 示例 typo — 已修正
memory_usage_溢出 —AddNeighbor中已添加 overflow checkUpdateId锁范围 — 已缩小为仅包裹 conjugate graph 更新
代码质量评估:
- 新文件
hgraph_enhance.cpp结构清晰,错误处理完善 - 序列化支持完整(binary + streaming),版本兼容性处理得当
- 测试覆盖全面:基本功能、并发安全、range search 不受影响、参数解析、序列化往返
- 内存统计正确集成到
cal_memory_usage()和GetMemoryUsageDetail() - 参数映射遵循现有模式,
CheckCompatibility正确包含use_conjugate_graph
没有发现新的阻塞性问题。
3311525 to
f27fbe9
Compare
f27fbe9 to
a696d62
Compare
|
The failed Test Example X86 job is unrelated to this PR and is reproducibly flaky:
No PR code change is warranted. I am rerunning only failed job 97400471144. |
|
Control confirmation: I also built and ran the example 100 times from the PR base / current |
Signed-off-by: Xiangyu Wang <wxy407827@antgroup.com> Assisted-by: Codex:gpt-5
a696d62 to
ac5023e
Compare
Summary
Feedback/PretrainsupportValidation
Fixes: #2650